Skip to content

Rule: Add a new rule to check constructor for OM #48

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 6 commits into from

Conversation

jissereitsma
Copy link
Contributor

@jissereitsma jissereitsma commented Jul 14, 2018

Description

Rule to see if Object Manager is injected via the constructor.

Sniff checklist

  • Sniff is accompanied by unit test
  • Sniff is documented using the documentation template (FooSniff.md next to FooSniff.php)

@lenaorobei
Copy link

Just thought about this rule.
For MEQP we also rely on class name (variable name) as a string. This is the reason why we cannot mark such sniff with severity 10, because it's not 100% guarantee that variable $objectManager is actually an instance of ObjectManager.
PHP Codesniffer allows adding bootstrap file. In this case, there will be an opportunity to check if the specified class is instanceof ObjectManager.

@jissereitsma
Copy link
Contributor Author

Thanks @lenaorobei for chiming in! Yeah, perhaps in my PR the warning level is much too high. We have decided earlier to discuss the warning levels of all (new and existing) rules to see whether we agree upon that. Work in progress there.

As for the technical strategy of this rule: I know about the MEQP rule, which scans for the usage of the OB. However, somebody could bypass this by using a variable $god or something. Using my approach, at least you can see the full class name, using the Reflection API to load everything. Really cool. But the downside is that it requires the loading of the autoloader, so Magento needs to be present for this to work. Otherwise, the Reflection API gives an exception and the rule doesn't continue with checking things. In short, this rule requires the presence of Magento itself.

What is missing is indeed a solid check to check up instances. Once the bootstrap file that you suggested is in place, the usage in this role of the Reflection API opens up for many cool abilities: For instance, you can scan for deprecated methods, usage of deprecated classes, quickly load other classes, etc. But we'll need a discussion first to see if this is the way to go ..

@schmengler
Copy link
Collaborator

What is missing is indeed a solid check to check up instances. Once the bootstrap file that you suggested is in place, the usage in this role of the Reflection API opens up for many cool abilities: For instance, you can scan for deprecated methods, usage of deprecated classes, quickly load other classes, etc. But we'll need a discussion first to see if this is the way to go ..

It's tempting, I'd also like to have these features (checking for @api is also useful for example). But IMHO we should stand by the decision we made, to not depend on a Magento installation to be present.

For things like the object manager, it's possible to resolve class names in constructor arguments (or in getInstance() classes to fully qualified class names. Again, a AST based static analysis tool (PHPStan?) would be better suited than the token based PHPCodeSniffer, but as long as we use the latter, your reflection approach seems reasonable.

I've already thought about usage of deprecated classes, and a possibility would be to hard code those into a sniff, similar to the ForbiddenFunctions sniff. There could be separate sniffs for each 2.x branch of Magento which are then included in the corresponding rulesets.

@jissereitsma
Copy link
Contributor Author

@schmengler My problem with the earlier decision to not depend on Magento is that it does make sense from a point of view of "static code analysis". However, using Magento logic in rules (ObjectManager itself, Reflection, version detection) opens up to so many cases which aid extension quality (which is still the final goal) that I would propose the rephrase the earlier decision:

What if we say that the PHPCS rules do not require the presence of Magento. However, some rules can only easily be built when Magento is available. So perhaps, these rules should not break when Magento is not present. But when Magento is present, these rules could be "upgraded" to report much more valuable things.

Some pros & cons: It would still be possible to run PHPCS rules quickly on a code base to get a quick result (just like unit tests should be run fast). But longer, more thorough tests could be run on an instance with Magento available (just like integration tests add more value as well). This would also mean that the Travis CI that we currently use is fine for basic rules, but should be extended to run Magento instances as well (with various versions) to guarantee that rules work in various environments. This requires more work. But the analysis is something I think we would like to have somehow. The question is how to interpret this word "somehow".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants